-
Notifications
You must be signed in to change notification settings - Fork 1k
Implement AuthenticationHandler for custom auth mechanisms
#1072
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Implement AuthenticationHandler for custom auth mechanisms
#1072
Conversation
6ec779a to
fb6703a
Compare
CredentialProvider with a more powerful AuthHandlerAuthenticationHandler for custom auth mechanisms
fb6703a to
55d7dfb
Compare
|
(This PR is a bit large than the other one you opened, I'll take a look later) |
|
Yeah, the PR is a bit too large for my taste too. :) I didn’t mean for it to be merged as-is, more as a starting point for discussion around the issues I opened. Happy to split it up once we have a clearer idea of what we want to do for each one! |
c650a68 to
8c72c7d
Compare
lance6716
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
6 / 11 viewed, will review later
9720dc2 to
12a0543
Compare
lance6716
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
rest lgtm
dveeden
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems ok on first glance.
|
Could we mix authentication methods? E.g. a user having two authentication methods:
This could be useful for migrating from one to another. Or when a authmethod has limitations (like auth_socket). Or it could be used for MFA |
|
I’m not entirely sure, but I don’t think this is feasible. As discussed in #1069:
For additional context: I previously tried to implement a server that stored credentials for both From what I remember of these experiments, the only potentially viable strategy was for the server to announce Given that, I’d prefer not to allow multiple authentication plugin names per user if we can’t guarantee that this will work end-to-end. Exposing this would likely be misleading for developers playing with MFA is out-of-scope for me here. |
|
@ramnes Yes, I agree with that. Note that newer MySQL clients are able to do both native and caching_sha2. There was a macOS specific issue that broke that, but that was fixed: Homebrew/homebrew-core#201853 Newer MySQL versions did remove the server part of |
12a0543 to
5c8bd7c
Compare
|
Just double check even if you can't implement "a server that stored credentials for both mysql_native_password and caching_sha2_password", this PR is still needed? My only comment is #1072 (comment) |
|
Yes, indeed! The original idea was to support multiple passwords per user, e.g. for API tokens, or rotating secrets. |
f88af71 to
5f1f7c6
Compare
Hey team,
Here is some work around #1069 and #1071 to allow a discussion based on an actual implementation.
I think the changes here are pretty obvious:
Credentialcan have multiple passwords now;OnAuthSuccessandOnAuthFailureto the newAuthenticationHandler.One side effect of the multiple passwords per user is that we can't have
Xorupdate inplace anymore, because otherwise we would XOR the password we received every time we compare it to a new hash in the list of hashes to try.About hashes – I moved the
Credentialpasswords hash at comparison-time to avoid paying the full price for every connection. Let's say you have 1000 passwords to match against but match after 5 comparisons: it's better to hash only 5 passwords, not 1000.